[helm] Si bytes calculation always floor fix #10044
Conversation
There was a problem hiding this comment.
thanks for contributing the fix! The two changes in this PR looks orthogonal to each other: fixing siToBytes and changing the store-gateway GOMEMLIMIT. Can you open a PR for each (perhaps repurpose this PR and open a new one)?
I'm not sure about the 0.9 multiplication of the memory request for GOMEMLIMIT. At the every least we need some comparisons running this side-by-side with the old version for example to prove the need for it.
| @@ -41,6 +41,7 @@ Entries should include a reference to the Pull Request that introduced the chang | |||
| * [BUGFIX] Fix how `fullnameOverride` is reflected in generated manifests. #9564 | |||
| * [BUGFIX] Fix `extraObjects` linting with helm lint by padding with an extra new line. #9863 | |||
| * [BUGFIX] Alertmanager: Set -server.http-idle-timeout to avoid EOF errors in ruler, also for zone aware Alertmanager #9851 | |||
| * [BUGFIX] Fix calculation of `mimir.siToBytes` and use floating point arithmetics, also multiply the memory request by 0.9 for store-gateway `GOMEMLIMIT` #10044 | |||
There was a problem hiding this comment.
| * [BUGFIX] Fix calculation of `mimir.siToBytes` and use floating point arithmetics, also multiply the memory request by 0.9 for store-gateway `GOMEMLIMIT` #10044 | |
| * [BUGFIX] Fix calculation of `mimir.siToBytes` and use floating point arithmetics. Also, multiply the memory request by 0.9 for store-gateway `GOMEMLIMIT` #10044 |
|
@edwintye Thank you for your contribution! Please update the PR following the feedback given in the comments. |
|
Thanks Dimitar for the suggestion. I didn't realize that the jsonnet variant didn't have a multiplication factor for |
|
The CHANGELOG has just been cut to prepare for the next release. Please rebase |
What this PR does
Changes the computation of
mimir.siToBytesto use floating point arithmetics.Additionally, adding a multiplication factor of
0.9to the store gatewayGOMEMLIMITcalculation. In the issue, it was suggested the multiplication factor to be0.8but after some testing and looking at other projects (such as prometheus) I settled on0.9. Looking at the examples, it is common that the memory request and limit will be different so being a more generous number0.9is probably fine - some would argue a straight translation is probably fine as well which is the case in the jsonnet version. I am not too sure how exactly the jsonnet version works but from what I can tell this change does create a drift on theGOMEMLIMITcalculation between helm and jsonnet.The tests in ci -
compare-helm-with-jsonnet/compare-manifeststo be exact - is failing but I can't get it to work and looking at the other PRs it looks like they have been failing for a while.Which issue(s) this PR fixes or relates to
Fixes #9780
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.